Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unspecified descriptor mediatypes MUST be ignored #759

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Unspecified descriptor mediatypes MUST be ignored #759

merged 1 commit into from
Mar 6, 2019

Conversation

jzelinskie
Copy link
Member

As per out discussions on the dev mailing list, this is the first step towards ensuring that storage and distribution of non-specified content can be experimented with while maintaining a standards-compliant registry.

cc @stevvooe

image-index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

image-index.md Outdated
@@ -40,7 +41,7 @@ For the media type(s) that this document is compatible with, see the [matrix][ma

Image indexes concerned with portability SHOULD use one of the above media types.
Future versions of the spec MAY use a different mediatype (i.e. a new versioned format).
An encountered `mediaType` that is unknown SHOULD be safely ignored.
An encountered `mediaType` that is unspecified MUST be ignored.
Copy link
Member

@cyphar cyphar Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit ambiguous what "unspecified" means here -- does it mean that the implementation is not aware of the media-type or it's not defined in the OCI spec? If the former, then I'd prefer to use the word "unknown" as before.

The reason for this nit is that you might have other media-types (like you are planning for the distribution-spec) that an implementation is aware of, but the lay-person reading of "an encountered mediaType that is unspecified MUST be ignored" is that you must ignore mediaTypes not defined in the OCI spec (and even then, the phrasing still sounds wrong to me).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An encountered `mediaType` that is not [OCI-specified](media-types.md) or [compatible][matrix] MUST be ignored.

Copy link
Member

@cyphar cyphar Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An encountered `mediaType` that is not [OCI-specified](media-types.md) or [compatible][matrix] MUST be ignored.

Whoa -- why would you want to disallow people from having their own media-types? I think that the wording should be:

An encountered mediaType that is unknown MUST be ignored.

Which would result in any out-of-spec mediaTypes being usable with any implementation (to the extent that they can be used without knowing what the media-type is).

The text you are proposing would disallow implementations from defining their own out-of-spec media-types (because any such implementation that didn't ignore their own media-type would not be following the OCI spec), which kind of defeats the purpose of them. The original text of this PR was just ambiguous, but that text is far less favourable. Not to mention I know of several organisations that are using their own media-types with OCI images, so such a change would be breaking them.

My understanding of the reasons for this patch are that you actually don't want to do this, so I'm quite confused why you're proposing text that makes it harder to have arbitrary media-types in distribution-spec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here is tricky.

The intention is that tools that are OCI compliant should not accidentally manipulate content that is being stored alongside it. They should all fail in the same way, which is to ignore them.

The tools that store other artifacts are not going to be OCI-compliant. To do so, they'd have to implement a bunch of container-specific details. The idea is that these tools work alongside OCI in a way that doesn't break things.

The changes we are making to the specs are to enable the flexibility to experiment with storing this content without disrupting OCI. The intention is not to make OCI support arbitrary content because it's fundamentally a container standard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, and I agree. I don't agree that the current wording you've proposed does this -- in fact in my reading it actually disallows third-party mediaTypes which is the opposite of what we both want.

I'm fine with @stevvooe's suggested wording:

An encountered mediaType that is unknown to the implementation but unspecified by OCI MUST be ignored.

or the simpler variant I included above:

An encountered mediaType that is unknown to the implementation MUST be ignored.

Which would result in mediaTypes that are unknown to an implementation (meaning that it's an "other artifact" that the implementation doesn't know how to manage) then it won't touch it.

manifest.md Outdated
@@ -60,6 +60,7 @@ Unlike the [image index](image-index.md), which contains information about a set
- [`application/vnd.oci.image.layer.nondistributable.v1.tar+gzip`](layer.md#gzip-media-types)

Manifests concerned with portability SHOULD use one of the above media types.
An encountered `mediaType` that is unspecified MUST be ignored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

@cyphar
Copy link
Member

cyphar commented Feb 7, 2019

Rejected (for now -- see my proposed wording to see what my concerns are).

Rejected with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Feb 7, 2019

Rejected? Based on what? Your conclusion is weird.

Are you requesting changes or outright rejecting this?

Rejected with PullApprove

@vbatts
Copy link
Member

vbatts commented Feb 7, 2019

I'm not a fan of MUST be ignored. They MUST be safe to ignore though

@stevvooe
Copy link
Contributor

stevvooe commented Feb 7, 2019

@cyphar Let's see a suggested wording that helps with the intent, rather than issuing a rejection. The interpretation of "unspecified" does include OCI. That is up to the interpreter. If someone has specified it elsewhere and they don't ignore it, then they are compliant. This is to prevent implementations from rejecting mediatypes from one vendor or another.

What if it was qualified like this:

An encountered `mediaType` that is unknown to the implementation but unspecified by OCI MUST be ignored.

I'm not a fan of MUST be ignored. They MUST be safe to ignore though

This is very odd language. How can we make a strong statement about safety?

We need strong language to ensure that future mediaTypes can be added without causing issues with backwards compatibility.

@cyphar
Copy link
Member

cyphar commented Feb 8, 2019

@stevvooe

An encountered mediaType that is unknown to the implementation but unspecified by OCI MUST be ignored.

That seems far more reasonable to me (and I agree with it).

Let's see a suggested wording that helps with the intent, rather than issuing a rejection.

I explained my concerns and provided suggested wording in this comment -- maybe you didn't see it?

The main objection I have is that the currently-proposed wording (at least to me) seems to disallow mediaTypes that are known to the implementation but are unspecified in the spec -- obviously this is not what any of us wants (which is why I'm against the patch). Of course, I do think that making out-of-spec mediaTypes no longer potentially subject to being broken is a good thing.

I would be fine with:

An encountered mediaType that is unknown to the implementation MUST be ignored.

or your suggested text (I'm not sure about the need to mention specified or unspecified mediaTypes -- if an OCI image-spec implementation doesn't support mediaTypes specified in the OCI spec then it isn't OCI-compliant anyway).

@jzelinskie
Copy link
Member Author

I moved forward with Aleksa's final wording because I'd rather have the intentions clear than be technically correct.

PTAL

Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cyphar
Copy link
Member

cyphar commented Feb 19, 2019

LGTM, thanks @jzelinskie. I'm not sure what's up with the CI failure -- for some reason we are golinting the entire Go source code? Something is definitely wrong there...

Approved with PullApprove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants